Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7571: do not activate terminal window upon settings modificaion #7887

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Oct 11, 2020

Summary of the Pull Request

Took this as an easy starter. The method IslandWindow::SetAlwaysOnTop is triggered once terminal settings are reloaded (in TerminalPage::_RefreshUIForSettingsReload flow). This method calls SetWindowPos without SWP_NOACTIVATE. As a result the window gets activated, the focus is set and the cursor starts blinking.

Added SWP_NOACTIVATE flag there and in few additional places in island windows. See a discussion below.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Added SWP_NOACTIVATE in all SetWindowPos calls from IslandWindow and NoClientIslandWindow (where it was missing). Please let me know if this is an overkill - it is not required to fix the issue, however seems a good practice, that might help if we decide to apply more settings immediately.

Validation Steps Performed

  • Only manual testing - please guide me to the relevant UT framework, if exists.
  • Trying to reproduce this with VS attached doesn't work - the window gets the focus in any case.
  • Tested as a standalone application, by modifying different settings (and comparing the results before and after the fix).
  • Checked with Spy++ that no WM_ACTIVATE / WM_SETFOCUS is thrown upon settings modification
  • Applied terminal resizing, toggling full screen and focus mode to check no regression was introduced.

@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 11, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch. Thanks a bunch for doing this!

@DHowett
Copy link
Member

DHowett commented Oct 11, 2020

Feel free to unmark this as a draft; it looks fairly complete to me! Are you participating in Hacktoberfest? I can mark these pull requests as eligible if so.

@Don-Vito Don-Vito marked this pull request as ready for review October 12, 2020 14:49
@Don-Vito
Copy link
Contributor Author

Feel free to unmark this as a draft; it looks fairly complete to me! Are you participating in Hacktoberfest? I can mark these pull requests as eligible if so.

Thanks! And thanks for reminding about Hacktoberfest - I signed up to it right now. Why not to plant a tree? 😄

@zadjii-msft zadjii-msft self-assigned this Oct 12, 2020
@DHowett DHowett added hacktoberfest-accepted Needs-Second It's a PR that needs another sign-off labels Oct 13, 2020
@zadjii-msft zadjii-msft removed their assignment Oct 13, 2020
@DHowett DHowett merged commit cb732a4 into microsoft:master Oct 13, 2020
@DHowett
Copy link
Member

DHowett commented Oct 13, 2020

Thanks again for the contribution!

DHowett pushed a commit that referenced this pull request Oct 19, 2020
Took this as an easy starter. The method IslandWindow::SetAlwaysOnTop is
triggered once terminal settings are reloaded (in
TerminalPage::_RefreshUIForSettingsReload flow). This method calls
SetWindowPos without SWP_NOACTIVATE. As a result the window gets
activated, the focus is set and the cursor starts blinking.

Added SWP_NOACTIVATE in all SetWindowPos calls from IslandWindow and
NoClientIslandWindow (where it was missing). Please let me know if this
is an overkill - it is not required to fix the issue, however seems a
good practice, that might help if we decide to apply more settings
immediately.

## Validation Steps Performed
* Only manual testing - please guide me to the relevant UT framework, if
  exists.
* Trying to reproduce this with VS attached doesn't work - the window
  gets the focus in any case.
* Tested as a standalone application, by modifying different settings
  (and comparing the results before and after the fix).
* Checked with Spy++ that no WM_ACTIVATE / WM_SETFOCUS is thrown upon
  settings modification
* Applied terminal resizing, toggling full screen and focus mode to
  check no regression was introduced.

Closes #7571

(cherry picked from commit cb732a4)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal v1.4.3141.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) hacktoberfest-accepted Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants